New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
StateTimeline: Treat second time field as state endings #84130
Conversation
@@ -496,6 +545,7 @@ export function prepareTimelineFields( | |||
}, | |||
}, | |||
}; | |||
changed = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the fix for #76869
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally and it looks like it addresses the escalation - it might be a good idea to sync up as a team around null handling as its been several months since we poked at this and it would be good for everyone to fully understand the 3 different null modes
} else if (endFieldIdx === -1) { | ||
endFieldIdx = i; | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember that we have handling were we have consider the first field to be the time field, does it make sense though to always consider the next found time field as the end field? (i.e. couldn't other time data be represented in user's data?)
I'm not sure this should be default behavior vs only apply in certain circumstances 🤷🏻♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e. couldn't other time data be represented in user's data?
technically, yes. but there is plenty of existing "auto" behavior that makes assumptions like "first string field", "all number fields", etc. to get it perfect in all cases we'd have to give explicit control to users over what each field in their response represents for each viz panel.
my gut feeling is that endTime is the most likely additional time field. if this turns out to be wrong, we can then expose options to override these assumptions and select fields explicitly, like we do in Trend, Candlestick, XYChart, etc.
['OK', undefined, null, undefined, 'NO_DATA', undefined, undefined, null], | ||
[undefined, 'ERROR', undefined, null, undefined, 'WARNING', null, undefined], | ||
]); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
yep. but i'm also secretly hoping to ride it out till uPlot v2 and we don't need field joining / frame alignment any more. then there is only |
Fixes https://github.com/grafana/support-escalations/issues/9636
Fixes #84127
StateTimeline panel currently requires either
null
values or a new state to terminate the previous state. however, splicing in the null values into the correct positions is pretty much impossible with our transformations. datasets often contain some form ofendTime
field to indicate state terminations. what this PR does is detects the presence of a second time field and splices in the null values correctly (using our belovedouterJoin
transformation).additionally, it fixes a regression i introduced in #83355 that removed
spanNulls: -1
handling from GraphNG. it turns out we still need this while we do the final frame joining in lower layers and this is the only way that StateTimeline prep is able to communicate theNULL_RETAIN
intent. we can remove this once we pull up the frame alignment up into the panels. i added a test to make sure we test the intended behavior and not thespanNulls
setting itself, which is an implementation detail.new gdev dashboard that shows off the new endTime detection and requires that frame joining +
NULL_RETAIN
works properly.also fixes another
spanNulls
-related bug...Fixes #76869